Repository navigation
fix(correct): error on --min-corrected with empty input on the legacy paths - #919
Conversation
… paths (parity with the chain/fgbio) The two legacy (non-chain) correct code paths computed ratio_kept = records_written / total_records and gated on `ratio_kept < min`. On empty input total_records is 0, so this divided 0.0/0.0 to NaN, and `NaN < min` is always false -- silently passing --min-corrected instead of erroring, while the chain path's check_min_corrected (and fgbio) correctly error in this case. Extract check_min_corrected as a shared pub(crate) helper (moved from the chain module into commands::correct, which the chain module already depends on) and route both legacy sites through it, so all three correct paths share one implementation of the empty-input semantics instead of three copies that can drift.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe change centralizes ChangesMinimum-corrected validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The command now reports an error when a positive minimum-corrected threshold is used with empty input, consistently across execution modes. The change is covered across the affected paths and is ready to merge. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #919 +/- ##
==========================================
+ Coverage 93.55% 93.60% +0.04%
==========================================
Files 301 301
Lines 150626 150664 +38
==========================================
+ Hits 140923 141022 +99
+ Misses 9703 9642 -61 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
What
The two legacy (non-chain)
fgumi correctexecution paths silently accepted--min-correctedon empty input. Both computedratio_kept = records_written as f64 / total_records as f64and gatedif ratio_kept < min; on empty inputtotal_records == 0, soratio_keptis0.0/0.0 = NaN, andNaN < minis alwaysfalse— so--min-correcteddid nothing. The chain path (check_min_corrected) and fgbio both error in this case, so the default (no---threads) path diverged from both.Fix
check_min_correctedinto a single sharedpub(crate)helper (insrc/lib/commands/correct.rs) that treatstotal_records == 0with a positiveminas an explicit error, and have the chain module import and reuse it instead of keeping its own copy — one implementation, no divergent duplicates.execute_single_thread_mode, and the currently-#[allow(dead_code)]execute_threads_mode, fixed for sibling parity) now call the shared helper and error on empty input, matching the chain path and fgbio.The
--metricswrite ordering is deliberately unchanged: fgbio writes the metrics TSV before the--min-correctedassert (CorrectUmis.scala:152 — "a ratio below--min-correctedwill cause a failure but all files will still be written"), and the legacy path already matches that.Tests
test_min_corrected_empty_input_single_threaded_mode(rstest) andtest_min_corrected_errors_on_empty_input_threads_mode— both legacy paths error on empty input + positive--min-corrected, and pass with non-empty/met input; verified red pre-fix.test_min_corrected_empty_input_chain_mode(rstest) — the live--threadschain path errors on empty input end-to-end.Full gate green (
cargo ci-test9960 passed / 31 skipped, plus fmt/lint/doc and--no-default-features/--all-features).Relationship to the chain-only cutover
This is a live-bug fix on the default legacy path. A separate PR in this series retires the legacy
correctpath entirely (chain-only), which will delete this code — at which point the chain path (already correct) is the only path. Landing this first fixes the live bug immediately; if the cutover lands first, this can be closed as moot.Risk: command output changes: none;
unsafechanges: none, and CLAUDE.md needs no update; memory bounds, queue capacity, and thread/backpressure policy changes: none.The shared
check_min_correctedhelper now rejects empty input when--min-correctedis positive across legacy and chain paths. Successful output and metrics ordering remain unchanged. Tests cover all execution paths.